Use pre-order stages ordering in EXPLAN output #26655

losipiuk · 2025-09-17T19:56:48Z

Description

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

testing/trino-benchmark-queries/src/main/resources/sql/trino/tpch/q17.sql

github-actions · 2025-10-09T17:03:28Z

This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack.

wendigo · 2025-10-09T17:11:51Z

@losipiuk ready for review?

Rename getSubStagesDeepPreOrder to getSubStagesDeep. Call spots did not really depend on any specific ordering of returned stages.

sourcery-ai · 2025-10-13T08:39:53Z

Reviewer's Guide

This PR refactors StagesInfo to replace the specialized pre- and post-order traversals with unified getSubStagesDeep (pre-order) and getSubStagesDeepTopological (topological) methods and updates all consumer code—ExplainAnalyzeOperator, QueryMonitor, and PlanPrinter—to use the new APIs, ensuring that EXPLAIN output uses a consistent pre-order stage ordering.

Sequence diagram for finding failed tasks in QueryMonitor using new traversal

sequenceDiagram
    participant QueryMonitor
    participant StagesInfo
    participant StageInfo
    participant TaskInfo
    participant TaskStatus
    QueryMonitor->>StagesInfo: getSubStagesDeep(outputStageId, true)
    loop for each StageInfo
        StagesInfo-->>QueryMonitor: StageInfo
        QueryMonitor->>StageInfo: getTasks()
        loop for each TaskInfo
            StageInfo-->>QueryMonitor: TaskInfo
            QueryMonitor->>TaskInfo: taskStatus()
            TaskInfo-->>QueryMonitor: TaskStatus
            QueryMonitor->>TaskStatus: getState()
            TaskStatus-->>QueryMonitor: TaskState
        end
    end

File-Level Changes

Change	Details	Files
Refactor stage traversal in StagesInfo to unified pre-order and topological methods	Rename getSubStagesDeepPreOrder to getSubStagesDeep and adjust its overload Rename getSubStagesDeepPostOrder to getSubStagesDeepTopological and implement it using a builder, visited set, and reverse logic Remove or update old collectSubStageIdsPreOrder/PostOrder methods and fix their generic types	`core/trino-main/src/main/java/io/trino/execution/StagesInfo.java`
Update consumers to call new traversal methods	Replace getSubStagesDeepPreOrder/getSubStagesDeepPostOrder calls with getSubStagesDeep/getSubStagesDeepTopological Extract StagesInfo into a local variable in ExplainAnalyzeOperator and update state checks Adjust textDistributedPlan in PlanPrinter and failure detection in QueryMonitor to use topological ordering	`core/trino-main/src/main/java/io/trino/operator/ExplainAnalyzeOperator.java` `core/trino-main/src/main/java/io/trino/event/QueryMonitor.java` `core/trino-main/src/main/java/io/trino/sql/planner/planprinter/PlanPrinter.java`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

In findFailedTask you’ve switched from post-order to pre-order traversal, which may report a different failed task than before—double-check that this still aligns with the intended deepest-first behavior.
The private helper collectSubStageIdsPreOrder is now used for generic deep traversal but still named “PreOrder”—consider renaming it (and its parameters) to better reflect its updated purpose.
The topological traversal builds the list then calls reverse; you could simplify by adding elements in reverse order during the walk to avoid the extra reverse step.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In findFailedTask you’ve switched from post-order to pre-order traversal, which may report a different failed task than before—double-check that this still aligns with the intended deepest-first behavior.
- The private helper collectSubStageIdsPreOrder is now used for generic deep traversal but still named “PreOrder”—consider renaming it (and its parameters) to better reflect its updated purpose.
- The topological traversal builds the list then calls reverse; you could simplify by adding elements in reverse order during the walk to avoid the extra reverse step.

## Individual Comments

### Comment 1
<location> `core/trino-main/src/main/java/io/trino/execution/StagesInfo.java:136` </location>
<code_context>
-        collectSubStageIdsPostOrder(stageInfo, subStagesIds);
-        if (includeRoot) {
-            subStagesIds.add(root);
+        if (visitedFragments.contains(stageId)) {
+            return;
         }
</code_context>

<issue_to_address>
**question:** Cycle detection is introduced but not documented.

Using 'visitedFragments' avoids infinite recursion, but if cycles are unexpected, this could hide data problems. Consider raising an error or warning when a cycle is detected.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2025-10-13T08:39:54Z

core/trino-main/src/main/java/io/trino/execution/StagesInfo.java

-        collectSubStageIdsPostOrder(stageInfo, subStagesIds);
-        if (includeRoot) {
-            subStagesIds.add(root);
+        if (visitedFragments.contains(stageId)) {


question: Cycle detection is introduced but not documented.

Using 'visitedFragments' avoids infinite recursion, but if cycles are unexpected, this could hide data problems. Consider raising an error or warning when a cycle is detected.

ebyhr · 2025-10-14T01:23:42Z

@losipiuk Could you update "Release notes" section?

cla-bot bot added the cla-signed label Sep 17, 2025

martint reviewed Sep 17, 2025

View reviewed changes

testing/trino-benchmark-queries/src/main/resources/sql/trino/tpch/q17.sql Show resolved Hide resolved

losipiuk force-pushed the lukaszos/use-pre-order-stages-ordering-in-f26809 branch from 7b5b5de to b94e6fc Compare September 18, 2025 11:29

wendigo approved these changes Sep 18, 2025

View reviewed changes

github-actions bot added the stale label Oct 9, 2025

github-actions bot removed the stale label Oct 10, 2025

losipiuk force-pushed the lukaszos/use-pre-order-stages-ordering-in-f26809 branch from b94e6fc to 605de60 Compare October 13, 2025 08:28

losipiuk added 4 commits October 13, 2025 10:30

Remove unneeded getSubStagesDeepPostOrder

1f17701

Rename methods

bbc9e5b

Rename getSubStagesDeepPreOrder to getSubStagesDeep. Call spots did not really depend on any specific ordering of returned stages.

Add missing type annotation

af814d4

Use topological stages ordering in EXPLAIN output

0782d71

losipiuk force-pushed the lukaszos/use-pre-order-stages-ordering-in-f26809 branch from 605de60 to 0782d71 Compare October 13, 2025 08:38

losipiuk requested a review from kasiafi October 13, 2025 08:38

losipiuk marked this pull request as ready for review October 13, 2025 08:38

sourcery-ai bot reviewed Oct 13, 2025

View reviewed changes

kasiafi approved these changes Oct 13, 2025

View reviewed changes

losipiuk merged commit 928fd0d into trinodb:master Oct 13, 2025
98 of 100 checks passed

github-actions bot added this to the 478 milestone Oct 13, 2025

ebyhr mentioned this pull request Oct 14, 2025

Add Trino 478 release note #26726

Closed

losipiuk added the no-release-notes This pull request does not require release notes entry label Oct 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use pre-order stages ordering in EXPLAN output #26655

Use pre-order stages ordering in EXPLAN output #26655

Uh oh!

losipiuk commented Sep 17, 2025 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot commented Oct 9, 2025

Uh oh!

wendigo commented Oct 9, 2025

Uh oh!

sourcery-ai bot commented Oct 13, 2025

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Uh oh!

sourcery-ai bot Oct 13, 2025

Uh oh!

Uh oh!

ebyhr commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

Use pre-order stages ordering in EXPLAN output #26655

Use pre-order stages ordering in EXPLAN output #26655

Uh oh!

Conversation

losipiuk commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

Uh oh!

github-actions bot commented Oct 9, 2025

Uh oh!

wendigo commented Oct 9, 2025

Uh oh!

sourcery-ai bot commented Oct 13, 2025

Reviewer's Guide

Sequence diagram for finding failed tasks in QueryMonitor using new traversal

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ebyhr commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

losipiuk commented Sep 17, 2025 •

edited

Loading